Seatbelt: fall back to policy-allowed cwd to avoid getcwd warnings - #688
Seatbelt: fall back to policy-allowed cwd to avoid getcwd warnings#688richiemsft wants to merge 2 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
When an explicit process.cwd (or the inherited host cwd) is not readable under the deny-by-default Seatbelt profile, the child shell's startup getcwd() walk fails and leaks noisy "cannot access parent directories" warnings from bash's shell-init / job-working-directory onto stderr. resolve_working_directory now honors an explicit working directory only when it is readable under the filesystem policy (within a readwrite/readonly path and not within a denied path, matched component-wise). Otherwise it launches from a policy-allowed directory (first readwrite, else first readonly, else /) and logs an informational diagnostic to the mxc log. The command still runs unchanged; only the launch directory changes, never the granted filesystem access. Adds unit tests for the allow/deny/fallback/tilde/component-boundary cases and an end-to-end characterization test asserting an out-of-policy cwd produces no getcwd noise while the command still succeeds. Updates the Seatbelt backend doc. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e4e66e58-7a38-4be3-ab78-32fa15b726ca
89b985e to
8f21b13
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/backends/seatbelt/common/src/seatbelt_runner.rs:722
- The first policy path is not guaranteed to be usable. Missing paths are only warned about (
config_parser.rs:358-373), and a broader deny can cover an allow (for example, allow/work/subplus deny/work), so this can select a nonexistent or denied cwd and makeCommand::spawnfail instead of trying a valid readonly path or/. Select the first expanded, existing directory that also passes the deny check.
let default = request
.policy
.readwrite_paths
.first()
.or_else(|| request.policy.readonly_paths.first())
…check Addresses PR review feedback: - A raw `profileOverride` replaces the generated profile, so the readwrite/readonly/denied fields are not applied. resolve_working_directory now preserves an explicit cwd unchanged when an override is present instead of consulting the ignored policy fields (which could wrongly rewrite the cwd to the fallback and break relative commands). - is_working_directory_allowed now lexically normalizes (folds `.`/`..`, collapses repeated slashes) the requested cwd and the policy roots before the containment test, so `/work/../private` is no longer classified as within an allowed `/work`. Adds unit tests for both cases plus normalize_path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e4e66e58-7a38-4be3-ab78-32fa15b726ca
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/backends/seatbelt/common/src/seatbelt_runner.rs:788
- Lexical normalization does not compare filesystem aliases. On macOS, for example, a policy may allow
/tmp/workwhile a caller supplies the equivalent canonical cwd/private/tmp/work/sub; this check classifies that readable cwd as outside the policy and replaces it with/tmp/work, so commands using relative paths run from the wrong directory. Resolve both the cwd and policy paths through symlinks (including existing ancestors for absent tails) before applying the component-wise containment check.
/// Lexically normalize an absolute path: collapse repeated slashes, drop `.`
/// components, and resolve `..` by popping the previous component (never past
/// the root). Purely lexical — it does not resolve symlinks or touch the
/// filesystem — which is sufficient to fold the `..` a caller may embed in a
/// requested cwd before the containment check.
src/backends/seatbelt/common/src/seatbelt_runner.rs:723
- The fallback is not guaranteed to be policy-allowed: it blindly selects the first read-write/read-only entry. A valid policy such as
readwritePaths: ["/work/private"]withdeniedPaths: ["/work"]leaves that first entry denied by Seatbelt's trailing deny rule, so an out-of-policy cwd still launches in an unreadable directory and retains thegetcwdwarnings. The selected candidate should be checked against deny precedence (and skipped when unusable) before falling back to/.
This issue also appears on line 784 of the same file.
/// Pick a directory the Seatbelt profile is guaranteed to allow: the first
/// readwrite path, else the first readonly path, else `/`.
fn policy_fallback_directory(request: &ExecutionRequest) -> String {
docs/macos-support/seatbelt-backend.md:236
- This description contradicts the implemented
profileOverridebehavior: with an override, an explicit cwd is always honored because the filesystem fields are ignored, even when it is outside every read/write path. Document that exception so users of raw profiles do not expect the cwd to be replaced.
If `process.cwd` is provided it is honored **only when it is readable under the
filesystem policy** — i.e. within a `readwritePaths`/`readonlyPaths` entry and
not within a `deniedPaths` entry. If `process.cwd` is omitted, or points at a
directory the policy does not allow, it resolves to `readwritePaths[0]`, else
| /// ordering). Policy paths are tilde-expanded exactly as the profile builder | ||
| /// expands them so the comparison sees the same absolute paths the profile | ||
| /// grants. Matching is component-wise, so `/data` never matches `/database`. | ||
| fn is_working_directory_allowed(request: &ExecutionRequest, dir: &str) -> bool { |
There was a problem hiding this comment.
Relative working directories are incorrectly replaced by the fallback
process.cwd currently accepts relative paths, which Command::current_dir resolves against the executor's host cwd. This check instead compares the unresolved relative string directly with absolute policy paths, so it always considers the path unreadable.
For example:
{
"process": {
"cwd": "src"
},
"filesystem": {
"readwritePaths": ["/repo"]
}
}If the executor is running from /repo, the intended child cwd is /repo/src. The new check compares src with /repo, fails, and launches the child from the fallback /repo. Commands using cwd-relative files then run from the wrong directory.
Similarly, cwd: "." can be replaced by the first read-write path even when the inherited cwd is already inside an allowed read-only path.
Could we resolve relative cwd values against std::env::current_dir() before normalization and containment checking? Please also add regression tests for cwd: "." and cwd: "src" when their resolved absolute paths are policy-readable.
| /// grants. Matching is component-wise, so `/data` never matches `/database`. | ||
| fn is_working_directory_allowed(request: &ExecutionRequest, dir: &str) -> bool { | ||
| // Expand `~` exactly as the profile builder does, then lexically fold `.` / | ||
| // `..` so the containment test compares the path the kernel actually |
There was a problem hiding this comment.
The cwd readability check does not match the generated Seatbelt profile
is_working_directory_allowed only considers readwritePaths and readonlyPaths, but the generated Seatbelt profile also grants read access to several system locations unconditionally, including:
/bin/usr/bin/usr/lib/usr/share/System/Library- selected paths under
/private
This means an explicit cwd such as /usr/share or /Library/Application Support may be readable under the actual profile, but this check still rejects it and silently launches from the first configured read-write or read-only path instead.
That changes command behavior unnecessarily—for example, pwd, relative arguments, and tools that discover configuration from the cwd will see a different directory even though the requested directory was already accessible.
Could we derive this decision from the same baseline path data used by profile_builder rather than maintaining a narrower second definition of readability? A regression test using a baseline-readable cwd such as /usr/share would also help prevent the two implementations from drifting.
To be fair, filesystem policy being deny-by-default means that we really need a way to 'include' the directories that are accessible by default on a given container platform, so that a) policy authors don't have to list them explicitly and b) the policy is still accurate. The policy 'spec' does allude to this, but we've not implemented it yet.
| fn is_working_directory_allowed(request: &ExecutionRequest, dir: &str) -> bool { | ||
| // Expand `~` exactly as the profile builder does, then lexically fold `.` / | ||
| // `..` so the containment test compares the path the kernel actually | ||
| // resolves. Without this, `/work/../private` would spuriously match an |
There was a problem hiding this comment.
Escape the requested cwd before writing it to the diagnostic log
The fallback message interpolates request.working_directory directly into a line-oriented log entry. Unix paths can contain control characters, including newlines, so a value such as:
/outside-policy
ERROR: sandbox initialization failed
would create what appears to be a separate, forged log entry. This does not cross the sandbox boundary because the cwd comes from the caller's configuration, but it can make diagnostics misleading or difficult to parse reliably.
Could we log the path using escaped/debug formatting, such as {:?} or escape_default(), so each fallback remains a single well-formed log line?
| // Expand `~` exactly as the profile builder does, then lexically fold `.` / | ||
| // `..` so the containment test compares the path the kernel actually | ||
| // resolves. Without this, `/work/../private` would spuriously match an | ||
| // allowed `/work` even though it resolves outside it. |
There was a problem hiding this comment.
Add coverage for the new fallback diagnostic
The new fallback behavior includes an operator-facing log message explaining why the requested cwd was replaced, but the tests only verify the returned directory. They do not verify that the diagnostic was emitted or that it identifies the requested and fallback paths.
Since these tests already use a buffered Logger, could we add assertions that:
- A rejected cwd logs both the requested directory and the selected fallback.
- An allowed cwd does not emit a fallback diagnostic.
- The logged paths are escaped so control characters cannot create additional log lines.
This would protect the primary diagnostic users have when a command unexpectedly starts in a different directory.
microsoft-github-policy-service
Bot
added
the
Needs-Author-Feedback
📖 Description
When an explicit process.cwd (or the inherited host cwd) is not readable under the deny-by-default Seatbelt profile, the child shell's startup getcwd() walk fails and leaks noisy "cannot access parent directories" warnings from bash's shell-init / job-working-directory onto stderr.
resolve_working_directory now honors an explicit working directory only when it is readable under the filesystem policy (within a readwrite/readonly path and not within a denied path, matched component-wise). Otherwise it launches from a policy-allowed directory (first readwrite, else first readonly, else /) and logs an informational diagnostic to the mxc log. The command still runs unchanged; only the launch directory changes, never the granted filesystem access.
Adds unit tests for the allow/deny/fallback/tilde/component-boundary cases and an end-to-end characterization test asserting an out-of-policy cwd produces no getcwd noise while the command still succeeds. Updates the Seatbelt backend doc.
🔗 References
Resolves the Mac issue from #611
🔍 Validation
Validated locally on a Mac. All green
✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes (see docs/pull-requests.md)📋 Issue Type
GitHub Actions runs the PR validation build automatically. The ADO pipeline
(
MXC-PR-Build) is the Azure version of the PR pipeline, kept in parity with the GitHubActions build; it runs on merge to
main, and Microsoft reviewers with write access can trigger iton a PR with
/azp run. See docs/pull-requests.md.If the
dependency-feed-checkcheck fails on a new dependency, the crate must be added tothe feed before the PR can pass. See docs/pull-requests.md
for the steps.
Microsoft Reviewers: Open in CodeFlow